Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop requesting room keys from other users #2982

Conversation

poljar
Copy link
Contributor

@poljar poljar commented Dec 14, 2022

Requesting forwarded room keys from other users is quite pointless since we don't accept them. This mainly produces unnecessary to-device traffic.

The main commit in this PR is the third one, where we finally remove the requesting of room keys from other users. The previous commits are refactors of the forwarded room key handling and unit tests validating the forwarded room key handling.

Fixes: element-hq/element-web#23766
Fixes: element-hq/element-web#24681


This change is marked as an internal change (Task), so will not be included in the changelog.

We have some tests that check if receiving a forwarded room key works.
These claim to use the same user id, but in fact they change the user id
in the last moment before the event is passed into the client.

Let's change this so we're always operating with the same user id.
We never accept such room keys, so there isn't a point in requesting
them.
@poljar poljar requested a review from a team as a code owner December 14, 2022 11:46
@poljar poljar added the T-Task Tasks for the team like planning label Dec 14, 2022
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks generally good to me, modulo a few suggestions/nits as below.

The logic in shouldAcceptForwardedKey seems a little bit magical and I'm not entirely convinced we have comprehensive tests for it: it might be nice to make sure we do. But I'll not insist if you need to move on to other things.

src/crypto/algorithms/megolm.ts Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Outdated Show resolved Hide resolved
src/crypto/algorithms/megolm.ts Show resolved Hide resolved
@richvdh
Copy link
Member

richvdh commented Feb 27, 2023

@t3chguy
Copy link
Member

t3chguy commented Feb 27, 2023

@richvdh would be worth updating the OP otherwise github won't auto-close

@poljar
Copy link
Contributor Author

poljar commented Mar 1, 2023

I think that this has addressed all the issues and I merged develop in.

@poljar poljar requested a review from richvdh March 1, 2023 12:50
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@richvdh richvdh added this pull request to the merge queue Mar 2, 2023
@richvdh richvdh removed this pull request from the merge queue due to a manual request Mar 2, 2023
@richvdh richvdh added this pull request to the merge queue Mar 2, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 2, 2023
@andybalaam
Copy link
Contributor

Adding back to the merge queue after the change that I am hoping fixes the quality metrics.

@andybalaam andybalaam added this pull request to the merge queue Mar 2, 2023
Merged via the queue into matrix-org:develop with commit cd526a2 Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
4 participants